Skip to content

Conversation

@sneakers-the-rat
Copy link
Collaborator

@sneakers-the-rat sneakers-the-rat commented Jan 27, 2026

fix #135

(just drafting, awkward middle state here, need to pull out all the scheduler awaiting stuff. might be best to just take it out of scheduler altogether)


📚 Documentation preview 📚: https://noob--136.org.readthedocs.build/en/136/

@codspeed-hq
Copy link

codspeed-hq bot commented Jan 27, 2026

CodSpeed Performance Report

Merging this PR will degrade performance by 21.95%

Comparing zmq-async (3ee3578) with main (5fff4b7)

Summary

⚡ 3 improved benchmarks
❌ 2 (👁 2) regressed benchmarks
✅ 2 untouched benchmarks
🆕 1 new benchmark
⏩ 7 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
👁 Simulation test_kitchen_sink_process[ZMQRunner-testing-kitchen-sink] 5.6 ms 6.7 ms -16.92%
🆕 Simulation test_kitchen_sink_run[ZMQRunner-testing-kitchen-sink] N/A 70.2 ms N/A
👁 Simulation test_long_add[ZMQRunner-testing-long-add] 5 ms 6.4 ms -21.95%
Simulation test_long_add[SynchronousRunner-testing-long-add] 4.6 ms 3.9 ms +17.77%
Simulation test_kitchen_sink_process[SynchronousRunner-testing-kitchen-sink] 4.3 ms 3.6 ms +18.57%
Simulation test_kitchen_sink_run[SynchronousRunner-testing-kitchen-sink] 44.3 ms 37.5 ms +18.3%

Footnotes

  1. 7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@coveralls
Copy link

coveralls commented Jan 27, 2026

Coverage Status

coverage: 87.438% (-0.05%) from 87.488%
when pulling 3ee3578 on zmq-async
into 5fff4b7 on main.

…ation, before any cleanup so this is still messy as all hell and the statefulness test is not working bc the stateless nodes aren't returning anything
@sneakers-the-rat
Copy link
Collaborator Author

ok perf regressions look like they are in part from an error being emitted during benchmarking, which would be bad. also it looks like there is plenty of overhead from the thread pool executor in the command node callbacks which i think i can avoid by making the ZMQRunner callbacks private and async - in general i need to make the callbacks private on both command node and zmqrunner

@sneakers-the-rat
Copy link
Collaborator Author

ok need to stop here for rn, but just confirming that the ZMQRunner long-add regression is an artifact because of different parts of the code being visible to the benchmarking instrumentation, but the ZMQ kitchen sink test is weirdly slower for reasons I think are related to the threadpoolexecutor. that is a reasonable trade because we should get perf gains when there is actually network i/o to do, which is the purpose of the zmq runner. will do profiling later on this.

@sneakers-the-rat
Copy link
Collaborator Author

the gains in the sync runner, on the other hand are real - from removing the Condition objects from the scheduler (where they weren't being used by the sync runner anyway)

@sneakers-the-rat
Copy link
Collaborator Author

alright i can’t really replicate this locally, but if you take a look at the codspeed metrics, something like 40% of the time is spent in a set_dealloc function which looks like an internal python method used to deallocate memory used by frames when rendering a traceback? if i remove that from the calculations then the numbers match what i see locally. I don’t see any tracebacks printed and there isn’t a place where they would be getting swallowed without being re-raised that I can see. i’m going to mark those regressions as acknowledged and move on since it is actually faster and this looks like it’s just some artifact from mysterious profiler instrumentation

when measuring locally, this is about twice as fast as the prior implementation, and 4x as fast as 74cf737 for some reason, not sure why Poller is so slow.

@sneakers-the-rat sneakers-the-rat marked this pull request as ready for review February 7, 2026 05:58
@sneakers-the-rat
Copy link
Collaborator Author

doing a merge since raymond is busy!

@sneakers-the-rat sneakers-the-rat merged commit 26e2a08 into main Feb 7, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

convert zmq runner to full async

2 participants